Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(android): update text natively only if needed (fixes selection menu missing) #36663

Closed
wants to merge 1 commit into from
Closed

fix(android): update text natively only if needed (fixes selection menu missing) #36663

wants to merge 1 commit into from

Conversation

hannojg
Copy link
Contributor

@hannojg hannojg commented Mar 27, 2023 •
edited
Loading

Summary

Problem

There exists a bug where on android when having a controlled TextInput using the selection prop the selection menu would not show up (the first TextInput is uncontrolled and there you can see its working):

Screen.Recording.2023-03-27.at.18.48.27.mov

There has also been an issue, which was closed as stale:

The reason for this behaviour is the following:

  • The user moves the selection cursor in the native EditText view
  • The native onSelectionChange listeners fires, which will call the JS onSelectionChange callback
  • The JS onSelectionChange callback will update the react state lastNativeSelectionState
  • When the TextInput is uncontrolled the selection that is passed to the native view will stay null
  • When the TextInput is controlled however a new object gets set as state for lastNativeSelectionState, causing the component to re-render.
  • The re-render causes the props to get applied, which then leads to a maybeSetText call (e.g. updating the text of the native view)
  • maybeSetText doesn't check if the text is the same, and updates the texts natively
  • This update causes the selection menu to disappear. There is no way to prevent this.

Solution

We don't want to update the text natively if it hasn't changed. There is already a check present in maybeSetText, but it only checks for text changes when the TextInput is a secure one (e.g. a password input).

I propose that we always check if the text has changed, and only update the text natively if it really did.
These changes can be seen in this PR.

With these changes you can see that in the tester app the text selection also works for controlled inputs:

Screen.Recording.2023-03-27.at.19.16.49.mov

Changelog

[ANDROID] [FIXED] - Selection menu missing when TextInput uses controlled selection prop

Test Plan

  • I see that there are some references to a test file called TextInputEventsTestCase, however I can't find it anywhere in the code. I would like to run those tests and confirm everything is still working as expected
  • Manual testing for the TextInput in the rn tester app

@hannojg
fix: update text only if needed
fda1081 
Not just in case of a secure text type
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 27, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,519,307 -105
android hermes armeabi-v7a 7,835,586 -121
android hermes x86 8,998,436 -115
android hermes x86_64 8,854,608 -107
android jsc arm64-v8a 9,140,603 +39
android jsc armeabi-v7a 8,332,819 +42
android jsc x86 9,194,267 +33
android jsc x86_64 9,453,358 +48

Base commit: 04df252
Branch: main

@hannojg hannojg mentioned this pull request Mar 28, 2023
Closed
@javache javache requested a review from mdvacca March 30, 2023 09:09
@javache
Copy link
Member

javache commented Mar 30, 2023

Comment from @mdvacca when this logic was first added:

reactTextUpdate is a Spannable and it might contain an HTML format, comparing only the text might create a regression when updating only the format of the content of a ReactEditText.
I believe that comparing text and format for each text update can be too expensive, so I would make this comparison ONLY when the secureTextEntry property is enabled (since I don't believe using html format into a password makes much sense)

@hannojg
Copy link
Contributor Author

hannojg commented Mar 31, 2023 •
edited
Loading

@javache Thx for clarifying that. And yes it's right I missed that we work with spannables here. So this code would introduce issues where the native text wouldn't update.

Example without this change:

Screen.Recording.2023-03-31.at.13.25.26.mov

Example with this change (broken):

Screen.Recording.2023-03-31.at.13.26.37.mov

I feel like the best solution to address this issue then is to avoid the re-render / the update of the native view when JS updates the selection state to the same as it is in native already.

@javache
Copy link
Member

javache commented Apr 4, 2023

I feel like the best solution to address this issue then is to avoid the re-render / the update of the native view when JS updates the selection state to the same as it is in native already.

Makes sense. Do you want to try and update maybeSetSelection to bail out early if the selection doesn't change to see if that works?

@hannojg
Copy link
Contributor Author

hannojg commented Apr 5, 2023 •
edited
Loading

Hm, unfortunately I don't think that would help, as maybeSetText would get executed anyway from the call in updateExtraData:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java

Lines 387 to 388 in de2f01d

view.maybeSetTextFromState(update);
view.maybeSetSelection(update.getJsEventCounter(), selectionStart, selectionEnd);

And the text being replaced is the reason for the selection menu to disappear. I am not sure if there is a feasible solution. We just can't really tell if two Spannables are identical from their content or not (we can, but the solution would be too expensive / complex to run on every re-render).

So it feels like the solution to keep the selection menu while manually controlling a TextInput is to use the imperative approach...

@s77rt s77rt mentioned this pull request Apr 13, 2023
Closed
@hannojg
Copy link
Contributor Author

hannojg commented May 31, 2023

Closed in favour of #37424 as it successfully fixed the issue without introducing regressions!

@hannojg hannojg closed this May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdvacca mdvacca Awaiting requested review from mdvacca

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hannojg @analysis-bot @javache @facebook-github-bot